feat(clerk-expo): Introduce support for LocalAuth with LocalCredentials#3663
feat(clerk-expo): Introduce support for LocalAuth with LocalCredentials#3663panteliselef merged 28 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 6b6308d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
44fb76b to
ecce02d
Compare
|
!snapshot |
|
Hey @panteliselef - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/clerk-expo@1.3.0-snapshot.ve946474 --save-exact
npm i gatsby-plugin-clerk@5.0.0-beta.45 --save-exact |
| return; | ||
| } | ||
|
|
||
| if (numericTypes.includes(AuthenticationType.FINGERPRINT)) { |
There was a problem hiding this comment.
❓If the device supports multiple authentication methods (fingerprint and facial recognition), are we prioritizing fingerprint here?
There was a problem hiding this comment.
That's a very nice question, tbh my intention here was to have AuthenticationType.IRIS behave like face-recognition so i might need to reverse the logic, in order for iris/face have priority
| export function LocalCredentialsProvider(props: PropsWithChildren): JSX.Element { | ||
| const { isLoaded, signIn } = useSignIn(); | ||
| const { publishableKey } = useClerk(); | ||
| const key = `__clerk_local_auth_${publishableKey}_identifier`; |
There was a problem hiding this comment.
💡Nit: Could we extract this interpolation to a separate function? Or perhaps type it, to avoid accidentally changing it or misspelling between individual variables.
| const [isEnrolled, setIsEnrolled] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| let ignore = false; |
There was a problem hiding this comment.
❓Is the purpose of ignore to not re-fetch isEnrolledAsync?
There was a problem hiding this comment.
So the intention is to avoid race conditions in a situation where the component gets unmounted but the promise resolves after the component is already unmounted.
|
!snapshot |
|
Hey @panteliselef - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/clerk-expo@2.1.0-snapshot.vff0337d --save-exact |
| biometryType: BiometricType | null; | ||
| }; | ||
|
|
||
| const LocalCredentialsInitValues: LocalCredentialsReturn = { |
There was a problem hiding this comment.
❓ Should we have something like isPlatformSupported in LocalCredentialsInitValues in order to be easier to check if the hook can be used on specific platforms like Web or a device that does not has any biometric support, as now it seems you will have to check if biometryType is null which is not so clear if you don't take a look at the code
There was a problem hiding this comment.
For devices that do not support biometric, android and iOS fallback to PIN, and if a pin does not exist then biometricType will be null.
with biometricType you get the available authenticator and it is useful mostly for UI purposes. Displaying a Face ID or a touch ID icon for example.
Maybe simply changing the name to supportedBiometricType would do ?
We don't wanna indicate that the device can use biometric, but instead if we can use local authentication (biometric or not) in order to store credentials.
There was a problem hiding this comment.
I was mostly referring for platforms that this will not work at all like web,the biometric support was just an example as I didn't know if this fallbacks to pin
brkalow
left a comment
There was a problem hiding this comment.
Need some JSDoc comments on exported methods, and left a few non-blocking comments. Nice work!
Feel free to dismiss if you address the comments during your day tomorrow.
| setUserOwnsCredentials(false); | ||
| }; | ||
|
|
||
| const authenticate = async () => { |
There was a problem hiding this comment.
💡 It would be nice to (optionally?) set the credentials after a successful auth
packages/expo/src/hooks/useLocalCredentials/useLocalCredentials.ts
Outdated
Show resolved
Hide resolved
…s.ts Co-authored-by: Bryce Kalow <bryce@clerk.dev>
packages/expo/src/hooks/useLocalCredentials/useLocalCredentials.ts
Outdated
Show resolved
Hide resolved
…s.ts Co-authored-by: Bryce Kalow <bryce@clerk.dev>
Description
This PR introduces a new hook API
Video of the new UX flow
RPReplay_Final1719504401.MP4
1.Example usage in a profile page
1.Example usage in a custom sign in flow
Checklist
npm testruns as expected.npm run buildruns as expected.Type of change